fix(compilers/openapi): stop silently dropping operation details - #272
Merged
Conversation
…licy The reserved-header-parameter code records why the compiler keeps such a parameter and reports it, but not why the report is unconditional. Invariant 6 sends anything inferred to an injectable policy that can be disabled, so the next reader reaching this code has to work out for themselves whether the omission is deliberate. It is: the three names are fixed by the specification rather than inferred from the document, the comparison is against a declared name, and the lowered document is identical whether or not the diagnostic fires — removing the call reddens only the diagnostic assertions, no golden or conformance IR. There is no inference to mark Inferred and no semantics to switch off. The severity choice is recorded alongside it, since error would both misstate a well-formed document and stop harness.Check before any later finding in the same spec.
This was referenced Aug 6, 2026
…operation-sweep # Conflicts: # docs/ir-design.md
The reserved-header report landed at one of the three positions OpenAPI states the rule. A header parameter named Accept, Content-Type or Authorization was reported (§4.8.12), but a Content-Type entry in a response's headers map (§4.8.17) or an encoding's (§4.8.15) still lowered in silence — the same deviation from the same SHALL, at the two positions the shared header lowering serves. Report all three under one code, renamed openapi/reserved-header-name since it is no longer parameter-specific. The headers-map half reports at the map entry's own pointer rather than the header object's: the reserved thing is the key a header is mapped under, so one component referenced from a reserved key and an ordinary one is two declarations, and only the reserved key is reported.
OpenAPI lets both a Path Item Object and an Operation Object declare servers, and says the operation's override the path item's. Only the path item's were read, so a document declaring both kept the superseded list under openapi:servers and dropped the effective one outright — an emitter reading that entry would route to a host the operation had replaced, with nothing reported either way. Keep the operation's own beside it under openapi:operationServers. They need separate keys because they are two declarations at two pointers: one key for both would make the surviving list depend on which lowering ran last, and do it silently. The path item's keeps the key it already shipped under. Preserved from lowerOperation rather than from each route, so no route added later can forget it — which is how the path-item half came to be missing on two of its three routes.
The operation-level servers test asserted one of the three routes that lower an operation, while the code comment claimed lowerOperation makes all three unmissable. Planting a guard that skipped the webhook and callback routes left the whole suite green — the same blind spot the path-item half of this pair was fixed out of. Assert all three routes with a distinct URL apiece, and witness the pair in the conformance corpus so the two-order oracle, irverify and the JSON round trip reach it. Deleting the fixture's override reddens TestConformance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Six places in the operation, parameter and content lowering were reported as dropping declared
source content with no IR representation and no diagnostic. I reproduced each one against the
current tree before touching anything — compiling a minimal spec and reading both the emitted IR
and the complete diagnostic list — because the report predates the compiler split and several
losslessness fixes have landed since.
mainbefore this changedescription,deprecated,style/explode,examples, extensions, content-form headers)styleandexplodealready reachedir.Property; those two were still dropped in silence.serverspathswalk; the other two routes produced neither anUnmodeledentry nor a diagnostic. Probing it also showed an Operation Object's ownserversread nowhere at all — the overriding declaration of the same keyword — so both are covered here.allowEmptyValueAccept,Content-Type,Authorization)Content-Typeentry in a response's and an encoding'sheadersmap. All three are covered here.ErrorCase.Typeand the media type went nowhere.$refTestComponentConstraints_RefSiblingKeepsThem). A componentBounded: {$ref: …, minimum: 5}interns withConstraints.Min == "5", matching what the property position does. No change here.The five that still reproduced share one mechanism: declared content that reaches the IR in no form
at all and says nothing about it. Each is now either lowered, kept verbatim under
UnmodeledwithReasonNoIRHomeand adegraded-constructdiagnostic, or — for the one case where keeping it asdeclared is the right answer — reported under a diagnostic code of its own.
1. Header
styleandexplode.explodedecides whether a collection-valued header goes onthe wire as one repeated field or one comma-joined value, so losing it left the IR unable to say
how the header serializes.
ir.Propertyhas a field for neither (ir.PartEncodingdoes, but thatis a multipart part's own config), so both are kept verbatim, the same way a parameter schema's
xmlhints already are. This fixes both callers of the shared header lowering — response headersand a multipart part's per-encoding headers — rather than only the response position it was noticed
at.
2. Path-item
serverson every route. A path item is one object with three parents. Theexisting
applyPathServersis now called from the webhook walk and the callback walk as well asfrom
paths, so a delivery-host override written on a webhook or a callback is kept and reportedexactly as the same declaration under
pathsalready was. No second helper was written.2b. An operation's own
servers, which override the path item's. OpenAPI lets both a PathItem Object and an Operation Object declare
serversand says the operation's override the pathitem's. Only the path item's were read, so a document declaring both kept the superseded list
under
openapi:serversand dropped the effective one outright: an emitter reading that entry wouldroute to a host the operation had replaced, and nothing reported it either way. Fixing item 2 alone
would have propagated that to two further routes.
The two are kept under separate keys — the operation's own under
openapi:operationServers—because they are two declarations at two pointers, and one map key cannot hold both without the
surviving list depending on which lowering ran last, silently. The path item's keeps the key it
already ships under, so no golden changes meaning. Preservation happens in
lowerOperationratherthan at each route, so no route added later can forget it, which is exactly how the path-item half
came to be missing on two of its three.
3.
allowEmptyValue. Its neighbours —style,explode,allowReserved, the content-stylemedia type — all reach
ir.HTTPParamBinding; this one has no field there and no reader anywhere.It is kept on
ir.Parameter, the carrier at this position that has anUnmodeledmap at all.Preservation keys off presence rather than truth:
allowEmptyValue: falseis a declared fact too,and recording only the
truespelling would be the compiler deciding which declarations count.4. Reserved headers — an explicit, documented policy, at every position that states it.
OpenAPI says a reserved header declaration SHALL be ignored in three places, not one: a parameter
with
in: headernamedAccept,Content-TypeorAuthorization(§4.8.12); aContent-Typeentry in a response's
headersmap (§4.8.17), whose media type the response's owncontentmapalready names; and a
Content-Typeentry in an encoding'sheadersmap (§4.8.15), which theencoding's own
contentTypedescribes separately. The compiler now keeps lowering all three andreports each under one new stable code,
openapi/reserved-header-name, at warning severity.Keeping the declaration is the deliberate choice: dropping declared content is a loss, and whether
to generate a header that collides with the security scheme, with content negotiation, or with the
media type the position already owns is an emitter's decision, not a compiler's. What was wrong was
making that deviation invisible — an emitter had no way to tell such a declaration from any other.
The comparison is case-insensitive, since HTTP field names are. The rationale is recorded on the
diagnostic code itself, which is where the next reader will reach it.
The two headers-map positions report at the map entry's own pointer rather than the header object's,
because the reserved thing is the key a header is mapped under: one component
$ref'd from areserved key and from an ordinary one is two declarations, and only the reserved key is reported.
That is the opposite choice from the
style/explodepreservation above, which keeps keywords theheader object itself writes and so records them at the declaration.
The report is unconditional rather than an
Optionsswitch, and that too is recorded on the code.Invariant 6 sends anything inferred to injectable policy, and nothing here is inferred: the names
are fixed by the specification, the comparison is against a declared name, and the lowered document
is identical whether or not the diagnostic fires — removing the call reddens only the diagnostic
assertions, no golden and no conformance IR. Warning rather than error because the document is
well-formed, and because
harness.Checkreturns at the first error diagnostic, which would hideevery later finding in the same spec.
5. Error-response
content, whatever its arity.ir.ErrorCaseholds a singleTypeRefand nomedia type, so an error declared only as
application/problem+jsonreached the IR indistinguishablefrom one declared as
application/json. One entry losing the key it was written under is the sameloss as several losing all but the first, so the content map is now kept in both cases, with the
diagnostic naming which of the two happened rather than one message covering both.
docs/ir-design.md's OpenAPI mapping row is updated for each of these, since it is normative aboutwhere a construct lands.
Scope
Nothing from the report was left out. Item 6 needed no change and is recorded above with the
evidence rather than quietly omitted. None of the five fixes mints an IR node, so no new ID
namespace is involved; the two-order oracle in
internal/harnesssweeps the whole conformancecorpus and stays green over the extended fixtures.
Probing turned up four further drops in the same walk that are not part of this change:
x-*extensions are read at no response at all, andlinkssurviveson a success response but not on an error one.
allowReservedandx-*reach no IR field, noUnmodeledentry andno diagnostic. Same mechanism as item 3 at a different object; kept out because the extensions
half overlaps openapi: response extensions dropped everywhere, links dropped on error responses #275 and the two should be settled together.
summaryanddescriptionare read nowhere. Held back deliberately:there is no
ir.PathItem, so it needs a recorded decision between merging into each operation'sDocsand preserving raw, and that is a lowering choice rather than the preservation this PRmakes.
Each is a separate rule from anything here, so each gets its own change rather than riding along.
Test plan
Fixtures were extended in the conformance corpus rather than added beside it, since none of these is
a new capability row:
header-content-schemagains a collection header declaringstyle/explode,webhooksandcallbackseach gain a path-itemserversoverride with a distinct URL,param-stylesgains anallowEmptyValueparameter, andper-status-errorsalready carried asingle-media error response. Unit tests in the
operationpackage pin the pointers and diagnosticmessages the goldens only record.
unwitnessed.golden.txtshrinks by one line:ErrorCase.Unmodeledhad no witness in the corpus before this.
webhooksadditionally declares an operation-levelserversoverride beside its path-item one, sothe corpus witnesses both keys of that pair and the two-order oracle,
irverifyand the JSON roundtrip all reach the case where they could collide.
The reserved-header rule is unit-tested rather than added to the corpus, since it changes only the
diagnostic stream: the header lowers identically either way, so a fixture would move a golden
without witnessing the report. Both
lowerHeaderscallers are covered, and the two-keys-one-component case pins which pointer the report lands on.
Every new behavioural assertion was checked by planting the defect back and watching it go red:
preserveHeaderSerializationiterates an empty keyword listTestConformance/header-content-schema,TestHeaders_SerializationKeywordsKept(both sub-cases)styleonly, droppingexplodestylealoneapplyPathServerscall removed from the webhook walkTestConformance/webhooks,TestOperations_PathItemServersKeptOnEveryRouteapplyPathServerscall removed from the callback walkTestConformance/callbacks,TestOperations_PathItemServersKeptOnEveryRouteapplyOperationServerscall removedTestOperations_OwnServersKeptBesideThePathItems,TestOperations_OwnServersSurviveBesideExtensionsopenapi:serverskeyTestOperations_OwnServersKeptBesideThePathItems,TestOperations_ServersKeysAreIndependent, so the key collision is pinned rather than assumedserverskeyword'sTestOperations_OwnServersKeptBesideThePathItemsop.UnmodeledTestOperations_OwnServersSurviveBesideExtensions, which exists because this mutation survived the rest of the suiteTestConformance/webhooks,TestOperations_OwnServersKeptBesideThePathItems. Both this and the webhook-only and callback-only variants; the test covered one route until this mutation survived the whole suiteserversdeleted fromwebhooks.yamlTestConformance/webhooks, confirming that fixture addition is load-bearing rather than inertfillErrorTypeguarded bycontent.Len() > 1againTestGolden,TestConformance/per-status-errors,TestConformance/component-reuse,TestConformance_UnwitnessedIRFields,TestErrorCase_SingleMediaTypeKeepsContentMap,TestDiag_SharedDeclarationReportsEachDefectOncepreserveAllowEmptyValuecall removedTestConformance/param-styles,TestParams_AllowEmptyValueKeptreservedHeaderParamDiagcall removedTestParams_ReservedHeaderNamesAreReportedreservedHeaderParamDiagreports unconditionally (no location guard, no name match)reservedHeaderEntryDiagcall removed fromlowerHeadersTestHeaders_ReservedContentTypeEntryIsReported,TestHeaders_ReservedContentTypeInEncodingIsReported,TestHeaders_ReservedNameIsTheKeyNotTheDeclarationreservedHeaderEntryDiagreports every entry (no name guard)==rather thanstrings.EqualFoldTestHeaders_ReservedNameIsTheKeyNotTheDeclaration, so the two-keys-one-component case is pinnedserversdeleted fromwebhooks.yamlTestConformance/webhooks, confirming the fixture addition is load-bearing rather than inertFull gate, in order:
gofmt -lclean,go vet ./...clean,golangci-lint run0 issues,go build ./...clean,./scripts/check-coverage.shpasses at 100% of statements.go run ./cmd/morphic-harnessover bothtestdata/conformance/openapiandtestdata/golden/openapireports
okfor every spec, which is what exercises the two-order diff,irverifyand the JSONround-trip over the extended fixtures.
Closes #39.